[EFM] Recoverable Random Beacon State Machine #6771

durkmurder · 2024-12-02T16:41:44Z

Context

The goal of this PR is to implement a state machine which allows to cover all possible cases which can happen when performing DKG. Previously our implementation was enforcing some of the rules but not all of them, what made things worse is that logic and interfaces wasn't explicit enough so an engineer can easily understand it. In this PR we have made an attempt to implement a robust approach to handling possible state and state transitions for both happy path and recovery path.

Some key points:

badger.RecoverablePrivateBeaconKeyStateMachine implements the state machine itself.
State machine transitions are initiated from dkg.ReactorEngine and dkg.BeaconKeyRecovery.
State machine doesn't care about origin of the key. It could be obtained from different sources(successful DKG or manually injected by operator) all that matters that key is committed. Caller needs to ensure that respective public key is part of the EpochCommit for respective epoch.

Link to the diagram which describes the state machine:

…DKG storage

…nd state but rather current

…internals of state machine

…tate machine

codecov-commenter · 2024-12-02T16:44:54Z

Codecov Report

Attention: Patch coverage is 1.88679% with 52 lines in your changes missing coverage. Please review.

Project coverage is 43.40%. Comparing base (7c71c41) to head (6ea64d6).

Files with missing lines	Patch %	Lines
cmd/consensus/main.go	0.00%	38 Missing ⚠️
model/flow/dkg.go	0.00%	10 Missing ⚠️
cmd/util/cmd/common/node_info.go	0.00%	3 Missing ⚠️
engine/common/grpc/forwarder/forwarder.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@                   Coverage Diff                    @@
##           feature/efm-recovery    #6771      +/-   ##
========================================================
+ Coverage                 41.71%   43.40%   +1.69%     
========================================================
  Files                      2030     1529     -501     
  Lines                    180459   141392   -39067     
========================================================
- Hits                      75285    61378   -13907     
+ Misses                    98978    75015   -23963     
+ Partials                   6196     4999    -1197

Flag	Coverage Δ
unittests	`43.40% <1.88%> (+1.69%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…m-beacon-state-machine

jordanschalm · 2024-12-04T03:13:46Z

engine/consensus/dkg/reactor_engine.go

@@ -154,24 +154,24 @@ func (e *ReactorEngine) startDKGForEpoch(currentEpochCounter uint64, first *flow
 		Hex("first_block_id", firstID[:]).        // id of first block in EpochSetup phase
 		Logger()

-	// if we have started the dkg for this epoch already, exit
+	// if we have dkgState the dkg for this epoch already, exit


Suggested change

// if we have dkgState the dkg for this epoch already, exit

// if we have started the dkg for this epoch already, exit

There are a few other instances of this replacement in this file. Assuming it's a rename gone wrong.

jordanschalm · 2024-12-04T03:27:55Z

engine/consensus/dkg/reactor_engine.go

@@ -427,10 +431,10 @@ func (e *ReactorEngine) end(nextEpochCounter uint64) func() error {
 			// has already abandoned the happy path, because on the happy path the ReactorEngine is the only writer.
 			// Then this function just stops and returns without error.
 			e.log.Warn().Err(err).Msgf("node %s with index %d failed DKG locally", e.me.NodeID(), e.controller.GetIndex())
-			err := e.dkgState.SetDKGEndState(nextEpochCounter, flow.DKGEndStateDKGFailure)
+			err := e.dkgState.SetDKGState(nextEpochCounter, flow.DKGStateFailure)
 			if err != nil {
 				if errors.Is(err, storage.ErrAlreadyExists) {


SetDKGState doesn't return this error type, it returns InvalidTransitionRandomBeaconStateMachineErr.

The comment above is outdated now, but it implies that we might already have persisted a failure state:

By convention, if we are leaving the happy path, we want to persist the first failure symptom

The state machine does not allow transitions from one state to itself (except for RandomBeaconKeyCommitted). If, as the current comment suggests, a failure state is already set at this point, we will throw InvalidTransitionRandomBeaconStateMachineErr as DKGStateFailure-> DKGStateFailure is not a valid transition. I don't think this is the case, but I'll return to this after reading further through the PR.

Suggestions:

update comment on lines 428-432

remove ErrAlreadyExists check

jordanschalm · 2024-12-04T03:34:40Z

model/flow/dkg.go

+	// Despite the key origin this is a terminal state which defines a safe Random Beacon key for the next epoch and allow node
+	// to participate in the Random Beacon protocol.


Suggested change

// Despite the key origin this is a terminal state which defines a safe Random Beacon key for the next epoch and allow node

// to participate in the Random Beacon protocol.

// Regardless of the key origin, this is a terminal state which defines a safe Random Beacon key for the next epoch and allows the node

// to participate in the Random Beacon protocol.

jordanschalm · 2024-12-04T03:35:09Z

storage/badger/dkg_state.go

+	flow.DKGStateStarted:          {flow.DKGStateCompleted, flow.DKGStateFailure, flow.RandomBeaconKeyCommitted},
+	flow.DKGStateCompleted:        {flow.RandomBeaconKeyCommitted, flow.DKGStateFailure, flow.RandomBeaconKeyCommitted},


Suggested change

flow.DKGStateStarted: {flow.DKGStateCompleted, flow.DKGStateFailure, flow.RandomBeaconKeyCommitted},

flow.DKGStateCompleted: {flow.RandomBeaconKeyCommitted, flow.DKGStateFailure, flow.RandomBeaconKeyCommitted},

flow.DKGStateStarted: {flow.DKGStateCompleted, flow.DKGStateFailure, flow.RandomBeaconKeyCommitted},

flow.DKGStateCompleted: {flow.RandomBeaconKeyCommitted, flow.DKGStateFailure},

jordanschalm · 2024-12-04T03:57:01Z

storage/badger/dkg_state.go

+var allowedStateTransitions = map[flow.DKGState][]flow.DKGState{
+	flow.DKGStateStarted:          {flow.DKGStateCompleted, flow.DKGStateFailure, flow.RandomBeaconKeyCommitted},
+	flow.DKGStateCompleted:        {flow.RandomBeaconKeyCommitted, flow.DKGStateFailure, flow.RandomBeaconKeyCommitted},
+	flow.RandomBeaconKeyCommitted: {flow.RandomBeaconKeyCommitted},


Why do we permit self-transitions for only this state?

jordanschalm · 2024-12-04T04:31:40Z

storage/dkg.go

+	RetrieveMyBeaconPrivateKey(epochCounter uint64) (key crypto.PrivateKey, safe bool, err error)
+}
+
+// DKGStateReader is a ready-only interface for reading state of the Random Beacon Recoverable State Machine.


Suggested change

// DKGStateReader is a ready-only interface for reading state of the Random Beacon Recoverable State Machine.

// DKGStateReader is a read-only interface for reading state of the Random Beacon Recoverable State Machine.

jordanschalm · 2024-12-04T04:38:10Z

storage/dkg.go

 	// No errors are expected during normal operations.
 	UpsertMyBeaconPrivateKey(epochCounter uint64, key crypto.PrivateKey) error
 }
+
+// InvalidTransitionRandomBeaconStateMachineErr is a sentinel error that is returned in case an invalid state transition is attempted.
+type InvalidTransitionRandomBeaconStateMachineErr struct {


Suggested change

type InvalidTransitionRandomBeaconStateMachineErr struct {

type InvalidDKGStateTransitionError struct {

The states are called flow.DKGState: I think we can use the more concise name here.

Minor nitpick on error naming convention: ErrXYZ for sentinels, XYZError for types.

jordanschalm · 2024-12-04T16:19:58Z

storage/badger/dkg_state_test.go

-			err = store.SetDKGStarted(epochCounter)
-			assert.NoError(t, err)
+		t.Run("-> flow.DKGStateStarted, should be allowed", func(t *testing.T) {
+			epochCounter++


Suggested change

epochCounter++

I'm guessing this was a previous strategy to ensure that all sub-tests use a different epoch counter. But currently each sub-test is calling setupState(), which uses a random epoch counter. Unless I'm missing something, I think we can remove.

jordanschalm · 2024-12-04T16:20:05Z

storage/badger/dkg_state_test.go

+		})
+
+		t.Run("-> flow.DKGStateFailure, should be allowed", func(t *testing.T) {
+			epochCounter++


Suggested change

epochCounter++

jordanschalm · 2024-12-04T16:22:14Z

storage/badger/dkg_state_test.go

+		epochCounter := setupState()
+
+		actualState, err := store.GetDKGState(epochCounter)
+		require.NoError(t, err, storage.ErrNotFound)


Suggested change

require.NoError(t, err, storage.ErrNotFound)

require.NoError(t, err)

durkmurder added 23 commits November 20, 2024 20:08

Changed structure of interfaces and corresponding implementation for …

100a92b

…DKG storage

Removed 'DKG started' from storage.

719b6a6

Updated DKG states to have extra states. They no more represent the e…

01bbdcf

…nd state but rather current

Updated usages of DKG storage in reactor engine

1d3c6a4

Added back GetDKGStarted for easier usage in reactor engine. Updated …

6c34886

…internals of state machine

Implemented allowed state transitions for recoverable random beacon s…

8aac526

…tate machine

Fixed unit test compilation. Updated allowed state transitions

064b651

Renamed interface methods

6bcaf38

Updated mocks

c13c7f6

Fixed tests for reactor engine

489c871

Updated godoc and reduced number of states for Recoverable state machine

10aab93

Updated usages of DKG state. Updated naming, godocs

fb28249

Removed flow.RandomBeaconKeyRecovered state. Cleanup

fb161f7

Updated how recovery happens in terms of inserting values

650b8b8

Implemented test for enforcing invariants of the uninitialized state

2386623

Added additional test cases.

bfe95b0

Updated logic for state transitions

79aac04

Added additional test for Completed state

1119c8c

Added tests for failure state

c8dfca6

Added extra tests for Random Beacon Key Committed state

c9182c5

Updated godoc for DKG tests

577712a

Godoc updates

7e728aa

Updated mocks

1fa11c6

durkmurder assigned jordanschalm and AlexHentschel Dec 2, 2024

durkmurder added 4 commits December 2, 2024 18:45

Naming updates

a92d8b7

Fixed broken tests

f0be4ba

Linted

62d399d

Fixed broken integration tests for DKG

bea9c1a

durkmurder added 6 commits December 3, 2024 16:23

Merge branch 'feature/efm-recovery' into yurii/6725-recoverable-rando…

89005a8

…m-beacon-state-machine

Fixed initialization of beacon private key state machine

0b9f732

Updated godoc. Added specific sentinel for error handling

5a98f76

Updated assertions with the account for sentinel errors

b639742

Updated logging

be18c57

Linted

d0df4fb

durkmurder marked this pull request as ready for review December 3, 2024 18:43

durkmurder requested review from AlexHentschel and jordanschalm as code owners December 3, 2024 18:43

durkmurder added 2 commits December 3, 2024 21:06

Fixed invalid exit logic in DKG reactor engine

0f98cf7

Fixed broken test

6ea64d6

jordanschalm reviewed Dec 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EFM] Recoverable Random Beacon State Machine #6771

[EFM] Recoverable Random Beacon State Machine #6771

durkmurder commented Dec 2, 2024 •

edited

Loading

codecov-commenter commented Dec 2, 2024 •

edited

Loading

jordanschalm Dec 4, 2024

jordanschalm Dec 4, 2024

jordanschalm Dec 4, 2024

jordanschalm Dec 4, 2024

jordanschalm Dec 4, 2024

jordanschalm Dec 4, 2024

jordanschalm Dec 4, 2024

jordanschalm Dec 4, 2024

jordanschalm Dec 4, 2024

jordanschalm Dec 4, 2024

	// if we have dkgState the dkg for this epoch already, exit
	// if we have started the dkg for this epoch already, exit

		// Despite the key origin this is a terminal state which defines a safe Random Beacon key for the next epoch and allow node
		// to participate in the Random Beacon protocol.

		flow.DKGStateStarted: {flow.DKGStateCompleted, flow.DKGStateFailure, flow.RandomBeaconKeyCommitted},
		flow.DKGStateCompleted: {flow.RandomBeaconKeyCommitted, flow.DKGStateFailure, flow.RandomBeaconKeyCommitted},

	// DKGStateReader is a ready-only interface for reading state of the Random Beacon Recoverable State Machine.
	// DKGStateReader is a read-only interface for reading state of the Random Beacon Recoverable State Machine.

	type InvalidTransitionRandomBeaconStateMachineErr struct {
	type InvalidDKGStateTransitionError struct {

	require.NoError(t, err, storage.ErrNotFound)
	require.NoError(t, err)

[EFM] Recoverable Random Beacon State Machine #6771

Are you sure you want to change the base?

[EFM] Recoverable Random Beacon State Machine #6771

Conversation

durkmurder commented Dec 2, 2024 • edited Loading

Context

codecov-commenter commented Dec 2, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durkmurder commented Dec 2, 2024 •

edited

Loading

codecov-commenter commented Dec 2, 2024 •

edited

Loading